-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eliminate needless lifetimes #723
Eliminate needless lifetimes #723
Conversation
I've been avoiding doing this to any repo as I think it is worse and loses useful information. I'd much rather just suppress it. There's an open issue in clippy about separating this or changing it as well. |
rust-lang/rust-clippy#13514 is that issue. |
I see your point, though I think it's fine for all the cases in this PR though? But I have no strong opinion here... |
I don't really want to suppress this lint, because it does give useful results for e.g. functions. That being said, these changes are also worse... rust-lang/rust-clippy#13286 seems to just be an unfortunate breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to be as close to the default as possible, but am still in agreement with other styles 😄
You mean those in this PR? So I think it comes down to sooner or later disable that lint or subdue to it (or add alternatives to clippy as suggested in this issue). Should I merge this or close it? |
Will leave it up to Daniel but in the other repos, my inclination is to suppress this rather than do these sorts of changes. Hoping that it gets fixed upstream. |
If you're going to be running nightly, you should probably supress it locally until the story on the clippy side becomes clearer. |
Sure that's not the issue, I can suppress it locally. So rather close this PR for now? As at least two people seem to be opposed, and I don't really care, but I like less syntax if possible (without too much loss of information). |
I think so; we should see what decision is made on the clippy side first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed my mind, I think these changes specifically are an improvement.
I think we need to be vigilant though; the example in the root of rust-lang/rust-clippy#13514 is definitely bad
So I guess in anticipation to updating the Rust version I should merge this? |
7dd18f7
to
f1179b2
Compare
Yeah, that would be good. |
Result from
__CARGO_FIX_YOLO=1 cargo clippy --fix
.As newest nightly clippy bleats about all of this.